quiche: quic client codec and setup integration test by danzh2010 · Pull Request #8496 · envoyproxy/envoy

danzh2010 · 2019-10-04T18:51:06Z

Implement EnvoyQuicClientStream|Session|Connection which are similar to server side code. EnvoyQuicClientConnection, as Network::ClientConnection, also dispatches IO events.

Use codec type HTTP3 and corresponding stats in QUIC codecs. Integrate QUIC server and client codec with HCM and ClientCodec via corresponding factory to avoid direct dependency on QUICHE code.

Risk Level: low, refactor to unused code
Testing: added quic_http_integration_test.cc and ConfigHelper::QUIC_HTTP_PROXY_CONFIG
Part of #2557

repokitteh-read-only · 2019-10-04T18:51:11Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8496 was opened by danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

include/envoy/http/protocol.h

Signed-off-by: Dan Zhang <danzh@google.com>

This reverts commit 3e39fbe. Signed-off-by: Dan Zhang <danzh@google.com>

Signed-off-by: Dan Zhang <danzh@google.com>

This reverts commit 95eb51d. Signed-off-by: Dan Zhang <danzh@google.com>

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 · 2019-10-10T03:06:52Z

Going to put this one in waiting until we sort out the HTTP/3 PR. This is amazing though. :)

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-11-25T19:26:46Z

Ping?

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Epic! Some small/random comments otherwise LGTM as something we can ship and iterate. @alyssawilk can you take another pass so we can get this merged and unblock @danzh2010? We have been very slow on reviews!

/wait

source/common/http/http3/quic_codec_factory.h

source/extensions/quic_listeners/quiche/codec_impl.cc

source/common/http/http3/quic_codec_factory.h

source/extensions/quic_listeners/quiche/codec_impl.h

source/extensions/quic_listeners/quiche/envoy_quic_alarm.h

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc

source/extensions/quic_listeners/quiche/envoy_quic_connection.cc

mattklein123 · 2019-12-03T01:00:21Z

source/extensions/quic_listeners/quiche/envoy_quic_server_session.cc


 EnvoyQuicServerSession::~EnvoyQuicServerSession() {
+  ASSERT(!quic_connection_->connected());
  QuicFilterManagerConnectionImpl::quic_connection_ = nullptr;


nit: del (similar elsewhere)

This is needed as explained in client session.

source/extensions/quic_listeners/quiche/envoy_quic_utils.cc

mattklein123 · 2019-12-03T01:02:44Z

test/config/utility.cc

            name: route_config_0
 )EOF";

+const std::string ConfigHelper::QUIC_HTTP_PROXY_CONFIG = BASE_UDP_LISTENER_CONFIG + R"EOF(


I wonder if this should be a config helper vs. a full new config? I will defer to @alyssawilk on this one.

I'd mildly prefer this to be a standard config modifier / helper, especially as eventually I'd like to run the H1/H2 tests with H1/H2/H3. Eventually I think we'll want to just apply the quic config modifiers when the client / upstream type is H3 and I think that'll be easier if we use standard config, so we don't get out of sync if someone tweaks the standard access loggers or some such.

Okay, added a TODO to generalize an HttpProxyConfig for both TCP and QUIC

alyssawilk

Yeah, I'm pretty much fine with this - I'm fine with any or all of my comments landing in a follow up just so we can get the big one merged.

alyssawilk · 2019-12-03T16:36:50Z

source/extensions/quic_listeners/quiche/envoy_quic_client_connection.cc

+  if (connectionSocket()->ioHandle().isOpen()) {
+    file_event_ = dispatcher_.createFileEvent(
+        connectionSocket()->ioHandle().fd(),
+        [this](uint32_t events) -> void { onFileEvent(events); }, Event::FileTriggerType::Edge,


Hand-waving here, we could have a periodic alarm which checked some runtime value (or check on packet ingress) which just disabled the listening socket. Again out of scope for this PR but I think it'd be good to be able to push via runtime and not just via LDS

source/extensions/quic_listeners/quiche/envoy_quic_client_connection.cc

source/extensions/quic_listeners/quiche/envoy_quic_client_session.h

alyssawilk · 2019-12-03T16:43:38Z

source/extensions/quic_listeners/quiche/envoy_quic_stream.h

+          // callback to do it in next loop. This is because unblocking QUIC
+          // stream can lead to immediate upstream encoding.
+          unblock_posted_ = true;
+          connection()->dispatcher().post([this] {


Just checking, if something else happens in this event loop to change should_block, switchStreamBlockState will still be called. Do we handle that correctly?

Yes, should_block_ will be updated in that case for switchStreamBlockState() to behave according to the latest value.

alyssawilk · 2019-12-03T16:46:11Z

test/config/utility.cc

            name: route_config_0
 )EOF";

+const std::string ConfigHelper::QUIC_HTTP_PROXY_CONFIG = BASE_UDP_LISTENER_CONFIG + R"EOF(


I'd mildly prefer this to be a standard config modifier / helper, especially as eventually I'd like to run the H1/H2 tests with H1/H2/H3. Eventually I think we'll want to just apply the quic config modifiers when the client / upstream type is H3 and I think that'll be easier if we use standard config, so we don't get out of sync if someone tweaks the standard access loggers or some such.

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

Again would like to see the integration test changes, but I'm happy with that being a follow-up

mattklein123

One small comment and then let's ship!

/wait

mattklein123 · 2019-12-04T19:14:43Z

source/common/http/http3/well_known_names.h

 public:
-  const std::string Client = "client_codec";
-  const std::string Server = "server_codec";
+  const std::string Client = "quiche_client";


We shouldn't let "quiche" leak into any core code. This should only be inside the extension itself, right? Can you move this into the extension directory or just remove this file entirely unless it needs to be referenced elsewhere?

I think we don't want any core code depends on extension code. That's why I add them here. These are used to create quic codec in CodecHelper and integration test, we can't remove them.

This is in the common directory. Just move this file into the extension directory then? There shouldn't be any reference to quiche in common code.

Current this codec name is hard coded in codec_client and codec_config. It's possible to pass down from config for different QUIC implmentations, but currently there is no need. So I added a TODO for that at the call sites.

Meanwhile the names here should indicate that we are using QUICHE implementation, but no need to differentiate client and server. So I changed them to one var "Quiche".

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Thanks!

mattklein123 · 2019-12-04T23:59:37Z

source/common/http/codec_client.cc

    codec_ = std::unique_ptr<ClientConnection>(
        Config::Utility::getAndCheckFactory<Http::QuicHttpClientConnectionFactory>(
-            Http::QuicCodecNames::get().Client)
+            Http::QuicCodecNames::get().Quiche)


nit: similar TODO here for config. Please add this in a follow up.

danzh2010 requested review from alyssawilk and mattklein123 as code owners October 4, 2019 18:51

merged all commits

edfbf4b

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the quicintegration branch from 481426d to edfbf4b Compare October 4, 2019 18:51

danzh1989 added 4 commits October 4, 2019 15:07

fix mem slice warning

f574a58

Signed-off-by: Dan Zhang <danzh@google.com>

remove broken include

5cc1622

Signed-off-by: Dan Zhang <danzh@google.com>

cryptoConnect

469dc2a

Signed-off-by: Dan Zhang <danzh@google.com>

fix xfcc test

68b6501

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 self-assigned this Oct 6, 2019

alyssawilk reviewed Oct 7, 2019

View reviewed changes

include/envoy/http/protocol.h Show resolved Hide resolved

danzh1989 added 2 commits October 7, 2019 11:41

remove stream libs

3a42546

Signed-off-by: Dan Zhang <danzh@google.com>

fix asan

c0ede9a

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk self-assigned this Oct 7, 2019

danzh1989 added 6 commits October 7, 2019 15:36

revert non-HCM change

3e39fbe

Signed-off-by: Dan Zhang <danzh@google.com>

Revert "revert non-HCM change"

d7c610b

This reverts commit 3e39fbe. Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quicintegration

013701b

Signed-off-by: Dan Zhang <danzh@google.com>

revert HCM, client and integration test

95eb51d

Signed-off-by: Dan Zhang <danzh@google.com>

Revert "revert HCM, client and integration test"

c4d2bad

This reverts commit 95eb51d. Signed-off-by: Dan Zhang <danzh@google.com>

remove unrelated change

e040f72

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added the waiting label Oct 10, 2019

danzh1989 added 8 commits October 10, 2019 11:49

fix asan

29128aa

Signed-off-by: Dan Zhang <danzh@google.com>

typo

31340f2

Signed-off-by: Dan Zhang <danzh@google.com>

add test for readDisable

88ebd06

Signed-off-by: Dan Zhang <danzh@google.com>

rename var

8a2740d

Signed-off-by: Dan Zhang <danzh@google.com>

typo

e43a1fe

Signed-off-by: Dan Zhang <danzh@google.com>

bufferLimit

438a8d7

Signed-off-by: Dan Zhang <danzh@google.com>

typo

4056e85

Signed-off-by: Dan Zhang <danzh@google.com>

add more

b6b10ac

Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 11 commits November 21, 2019 14:02

adjust client handshake idle timeout

e770972

Signed-off-by: Dan Zhang <danzh@google.com>

debug coverage test

5036296

Signed-off-by: Dan Zhang <danzh@google.com>

coverage test

0850f1a

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quicintegration

a28f76b

Signed-off-by: Dan Zhang <danzh@google.com>

fix client side

05f33c4

Signed-off-by: Dan Zhang <danzh@google.com>

fix interface change

3840dce

Signed-off-by: Dan Zhang <danzh@google.com>

adjust coverage log level

b52be05

Signed-off-by: Dan Zhang <danzh@google.com>

re-enable trace logging in coverage test

bcbc147

Signed-off-by: Dan Zhang <danzh@google.com>

revert bazel coverage script

b10639f

Signed-off-by: Dan Zhang <danzh@google.com>

disable quic logging

2855ae9

Signed-off-by: Dan Zhang <danzh@google.com>

revert handshake timeout to 5s

a2282a4

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quicintegration

bf2669e

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 requested changes Dec 3, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 3, 2019

alyssawilk reviewed Dec 3, 2019

View reviewed changes

danzh1989 added 2 commits December 3, 2019 19:55

address comments

88780bb

Signed-off-by: Dan Zhang <danzh@google.com>

revert bazelrc

88530b2

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Dec 4, 2019

alyssawilk previously approved these changes Dec 4, 2019

View reviewed changes

mattklein123 requested changes Dec 4, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 4, 2019

factory rename

1a42c05

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed alyssawilk’s stale review via 1a42c05 December 4, 2019 22:37

repokitteh-read-only bot removed the waiting label Dec 4, 2019

typo

cd58540

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 approved these changes Dec 4, 2019

View reviewed changes

mattklein123 merged commit 23724bc into envoyproxy:master Dec 5, 2019

danzh2010 mentioned this pull request Dec 6, 2019

quiche: add support of delayed close to QUIC session #9163

Merged

Conversation

danzh2010 commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 4, 2019

Uh oh!

Uh oh!

mattklein123 commented Oct 10, 2019

Uh oh!

danzh2010 commented Nov 25, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danzh2010 commented Oct 4, 2019 •

edited

Loading